Thsi is a WIP. I wonder if there's a way not to have acceptance tests run on every commit.

Thsi is a WIP. I wonder if there's a way not to have acceptance tests run on every commit.

I think we can skip test execution for draft PRs. As I understand it, defining all required types in https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target without synchronize type, won't run the test till you change it to ready_for_review. A similar approach with more control could be adding the condition in the job, something like:

if: ${{ github.event_name == 'pull_request_target' && github.event.action == 'synchronize' && !github.event.pull_request.draft }}

Thoughts on this @ctreatma ?

I do have, from time to time, drafts where I want to run E2E tests. Perhaps a label (No E2E Testing) or the title (WIP or NOE2E) could signal this?

you can still run it manually since that condition is only for github.event_name == 'pull_request_target' and not for workflow_dispatch

Yeah, @ocobles I think your suggestion is good. We should go with the second approach, adding a condition to the job (either by updating the authorize check or adding a separate check job before it). I don't think we should check for the synchronize action in the condition, though; if we want to limit the job to specific action names lets add that to the trigger config (synchronize and opened might be the only ones we really need?).

I'm now doing the migration of the provider, and I also want to move resources and datasources to subdirectories, e.g equinix/metal_ssh_key/{resource,datasource}.go

The framework provider needs to refer to owned resource [0], and if we have resource in subdir, it needs to import the subdir (see top of framework_provider.go).

I run into issues with the fact that we have a lot of helper code straight in the github.com/equnix/terraform-provider-equinix/equinix namespace. The code of a resource, for example in equnix/metal_ssh_key/resource.go needs to import the Config struct which is in github.com/equnix/terraform-provider-equinix/equinix and since the provider is importing the subdir package, there's an import cycle.

I am not sure how to resolve it. The natural solution would be to move the config (and all the helpers that resources and Config are using) to equinix/internal. Then both provider and resource code could use them without cycles. But that is a huge change, affecting every reference to Config or the helper functions, and should be agreed with other contributors. Also I think it should be done separately from the migration work.

We need to break the cycle provider@equinix -> resource@equinix/metal_ssh_key/ -> config_struct,helpers@equinix

It could be done without breaking things, if we copy all the necessary code to equinix/internal/. Then, existing unrelated code will not have to be changed along the migration, but if there will be work done in parallel (to the migration) on helpers in original location, it will be hard to merge once we get rid of the SDKv2 resources.

I think the helpers and config must be moved to a subdir eventually if we want to have resource code in subdirs. Or is there another solution in Go that I can't see? My suggestion is to move the helpers and config first, and continue with migration later.

Also, it's important to do the move of the config and helpers properly, so that really all the functionality (and its dependencies) that resources need goes to a subdir. It should be done somehow aware of the framework provider and resources in subdir. Along with the move to internal/, we should move one existing SDKv2 resource to a subdir so that we are sure that we restructured things properly.

I need your feedback @ctreatma @displague .

Thanks for reading this.

[0] https://github.com/equinix/terraform-provider-equinix/blob/framework_migr_base_plus_ssh_key/equinix/framework_provider.go#L93

@t0mk have a look at #142. I was fairly far along in the refactoring of directories . The cyclical imports I was running into (and had yet to resolve) were in test mode.

I wanted to keep equinix/ for the public Provider (name?) object so that other projects that import Terraform Go providers could do so.

I moved all resources and "helpers" into internal/ with separate packages for each.

@displague I see, you moved everything but provider.go provider_test.go and equinix_sweeper_test.go from <repo_root>/equinix to <repo_root>/internal and structured it a lot in there. Is it possible you wouldn't run into the cycle in tests if you'd move it to <repo_root>/equinix/internal? That's how I see it in Linode TP.

Anyway, it looks like a way to go, and too bad you didn't manage to merge it last year.

I don't think we need to move all resources to subdirs at this point, since we will migrate them to framework plugin ecventually. We just need one as a tracing bullet, to see that most of the helper code is separated from equinix namespace.

Give me green light on the code move and I will do a PR, restructure it in a reasonable way, and then we will discuss the paths and directory structure there, as that's not too relevant to the migration.

@t0mk, yes, you can go ahead with the code move. If you're looking for a resource / datasource to use as a testbed, the metal_ssh_key resource & datasource used here would be good candidates; step 1 would be refactoring within the SDKv2 implementation, and then step 2 would be migrating the refactored implementation to the plugin framework.

@ctreatma , great! I will do so. I was thinking about metal_ssh_key myself. Actually we don't even have a datasource for the user key.

My PR got too big too fast.
One piece that would be a nice chore that could be applied independently of resource moves and framework conversion is to move the helpers to various type specific packages.

Are there any disadvantages to moving into subfolders by service one level above the split by resource? I mean, internal/metal/ssh_key/ or internal/fabric/connection/. That approach keeps the main directory cleaner and helps limit the workspace for teams focused on a single service

Currently, we have a datasource for metal_project_ssh_key and resources for metal_project_ssh_key and metal_ssh_key. I see you have created a folder metal_ssh_key with both resource and datasource files. Is that datasource a new one to cover the GET user ssh key API request, or did you accidentally mix the resource for user ssh key and datasource for project ssh key?

Hi @t0mk, there are a few things I found while migrating resources that need to be addressed here:

• While they coexist, both provider schemas must produce the same output. So if you add a description to max_retries or max_retry_wait_seconds make sure it is included also in the old sdkv2 provider schema
• tfsdk struct tag doesn't have omitempty augmentation
• id attribute must be defined in the resource schemas
• A solution to add module to user agent:

//resource.go

func (r *Resource) Create(
	ctx context.Context,
	req resource.CreateRequest,
	resp *resource.CreateResponse,
) {

	r.Meta.AddFwModuleToMetalUserAgent(ctx, req.ProviderMeta)
	client := r.Meta.Metal

//config.go

func (c *Config) AddFwModuleToMetalUserAgent(ctx context.Context, meta tfsdk.Config) {
	c.Metal.UserAgent = generateFwModuleUserAgentString(ctx, meta, c.metalUserAgent)
}

func (c *Config) AddFwModuleToMetaGolUserAgent(ctx context.Context, meta tfsdk.Config) {
	c.Metalgo.GetConfig().UserAgent = generateFwModuleUserAgentString(ctx, meta, c.metalGoUserAgent)
}

func generateFwModuleUserAgentString(ctx context.Context, meta tfsdk.Config, baseUserAgent string) string {
	var m ProviderMeta
	diags := meta.Get(ctx, &m)
	if diags.HasError() {
		log.Printf("[WARN] error retrieving provider_meta")
		return baseUserAgent
	}
	if m.ModuleName != "" {
		return strings.Join([]string{m.ModuleName, baseUserAgent}, " ")
	}
	return baseUserAgent
}

more findings @t0mk :

• I think there's no schema URL validator in the framework and it seems documentation https://developer.hashicorp.com/terraform/plugin/framework/migrating/providers#framework-4 is wrong. You can create a custom validator but I would rather use RegMatches for the endpoint field

import "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"

...

var UrlRE = regexp.MustCompile(`^https?://(?:www\.)?[a-zA-Z0-9./]+$`)

...

"endpoint": schema.StringAttribute{
  Optional:    true,
  Description: "The Equinix API base URL to point out desired environment. Defaults to " + config.DefaultBaseURL,
  Validators: []validator.String{
    stringvalidator.RegexMatches(urlRE, "must be a valid URL with http or https schema"),
  },
},

that go test error after my last commit is related to https://developer.hashicorp.com/terraform/plugin/testing/migrating#flag-redefined-panic

I'm going to open a different PR to fix the code before continuing with the work here. So basically we need to replace any resource.StateChangeConf references with helper/retry package

@ctreatma definitely #551 worked as expected, I don't see that error anymore. Here's is the specific tests for both migrated resources https://github.com/equinix/terraform-provider-equinix/actions/runs/7744881780/job/21119662842#step:6:4306 https://github.com/equinix/terraform-provider-equinix/actions/runs/7744881780/job/21119662842#step:6:4405